Repository navigation
fix(propeller): clamp ArrayNode subNode delta timestamps instead of panicking - #8270
BhavayGoyal wants to merge 2 commits into
Conversation
…anicking The delta between the ArrayNode and a subNode start time was stored in SubNodeDeltaTimestamps unclamped. A subNode starting more than MaxDeltaTimestamp after the ArrayNode, or a negative delta after the ArrayNode attempt was retried or recovered (wrapping to a huge uint64), made CompactArray.SetItem panic on every evaluation, so the workflow could never make progress again. Saturate the delta to the range the array item can hold. Signed-off-by: Bhavay Goyal <bhavaygoyal4b@gmail.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #8270 +/- ##
==========================================
- Coverage 57.43% 57.35% -0.09%
==========================================
Files 925 931 +6
Lines 57772 58346 +574
==========================================
+ Hits 33184 33463 +279
- Misses 21583 21832 +249
- Partials 3005 3051 +46
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![cloud-verifier-agent[bot]](https://avatars.githubusercontent.com/u/79478050?s=60&v=4){kind=small}
There was a problem hiding this comment.
cloud-verifier · ⚠️ 1 finding, not blocking
🔴 Clamped delta makes subNode start time early; can trigger spurious timeouts · flytepropeller/pkg/controller/nodes/array/handler.go:419 · high · medium likelihood · see inline comment
❔ Not verified: CV-B-correctness/caller-contract
Reviewed 99a079999c · turn 1 · re-run with /cloud-verifier review
| } else if startedAt != nil && arrayNodeState.SubNodeDeltaTimestamps.GetItem(index) == 0 { | ||
| // otherwise if `SubNodeDeltaTimestamps` is unset, we compute the delta and set it | ||
| deltaDuration := uint64(subNodeStartedAt.Time.Sub(startedAt.Time).Seconds()) | ||
| deltaDuration := deltaTimestampSeconds(startedAt.Time, subNodeStartedAt.Time, arrayNodeState.SubNodeDeltaTimestamps.ItemSize) |
There was a problem hiding this comment.
🔴 Clamped delta makes subNode start time early; can trigger spurious timeouts
Clamping makes the rebuilt subNode start earlier than the real start, and the execution-deadline check uses that start. A subNode can time out early. Consider not enforcing the deadline from a clamped offset.
Tracking issue
Closes #8144
Why are the changes needed?
The ArrayNode handler stores each subNode's start offset (seconds since the ArrayNode's
LastAttemptStartedAt) inSubNodeDeltaTimestamps, aCompactArraysized byMaxDeltaTimestamp(3 days by default, so 18-bit items, max 262143). The value was written unclamped:So
SetItempanics (Value [267605] is too big. Max value is [262143].) when:uint64gives a huge value.The value is never stored, so the panic repeats on every round until the workflow runs out of system retries.
What changes were proposed in this pull request?
deltaTimestampSecondsinarray/handler.go, which returns the delta in whole seconds saturated to[0, 2^ItemSize - 1](the limitCompactArray.SetItemactually checks). It uses the array's ownItemSize, so it stays correct ifmax-delta-timestampis changed after the array was created.LastAttemptStartedAtin node events is less precise. A delta of 0 already means "not set" and gets recomputed on the next round.How was this patch tested?
TestDeltaTimestampSeconds(same time, in range, largest storable value, the 267605s value from the issue, subNode started before the ArrayNode). It checks the result and thatSetItemdoesn't panic on an array built with the default 3-day max. With the old expression the "beyond range" and "negative" cases panic; with the fix all pass.go test ./pkg/controller/nodes/array/passes, andgolangci-lint run ./pkg/controller/nodes/array/reports 0 issues.Labels
(Written with Claude, working with @BhavayGoyal.)